extending the epochal discrete CTMC model#1129
Closed
jsigao wants to merge 3 commits into
Closed
Conversation
…long a branch when that branch overlaps with both multiple epochal matrices and multiple epochal rates
…istories under either an epochal matrix model or an epochal rate model (or both)
msuchard
requested changes
May 28, 2022
Member
There was a problem hiding this comment.
@jsigao -- i am going to reject this pr for a couple of reasons that are straight-forward to fix:
- please target a development branch, like
hmc-clocksince this is a rather large change of behavior - pleaser remove massive amounts of code duplication (i.e. with the
getBranchRateModelMappingcopied across many files.
Contributor
Author
|
Dear @msuchard -- thanks very much for your review! I'll make the requested changes to this PR as soon as possible. |
Contributor
Author
|
Dear @msuchard -- sorry for the delay in making the requested changes. As the code-duplication removal affects many files, I've created a new pull request as a revised version of this PR to make the commit history a bit cleaner. To avoid potential confusion, I'll close this PR for now. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request contains three commits that extend the epochal discrete CTMC (substitution/dispersal) model. Specifically, this extension allows the transition-probability (P) matrix to be computed correctly along a branch that overlaps with multiple Q-matrix epochs (Bielejec et al., 2014) and multiple average-substitution/dispersal-rate epochs (Membrebe et al., 2019), and the stochastic-mapping function (the endpoint-conditioned uniformization algorithm; Hobolth and Stone, 2009) to work under the epochal model (either epochal matrices or epochal rates or both). A more detailed description about the extension can be found in S1 (S1.1 for the P-matrix computation and S1.2 for the stochastic-mapping function) of Gao et al., 2021.
The first commit (
94e91fe) added agetBranchRateModelMappingfunction to eachBranchRateModelclass (where the only non-trivial implementation is in theRateEpochBranchRateModelclass), imitating thegetBranchModelMappingfunction of theBranchModelclasses.In the second commit (
e86792c), this added mapping function is invoked in theSubstitutionModelDelegateclass very similarly as thegetBranchModelMappingfunction, allowing the average rate of each Q-matrix epoch that overlaps with a given branch to be computed correctly (instead of assuming that it is the average rate of the entire branch).The third commit (
9e81125) extends the endpoint-conditioned uniformization algorithm to work under the epochal model by (1) dividing a branch into pieces (where each piece only overlaps with one Q-matrix epoch and one average-rate epoch), (2) drawing the state at the end of each piece according to the probability conditioned on the start state of the piece and the end state of the branch, and (3) simulating history over each piece using the time-homogeneous algorithm.Please let me know if I failed to follow the preferred coding style in this extension or if some of the functionalities introduced here would be preferred to be implemented in an alternative way. Thank you in advance for reviewing this PR.